Conversation
…etheus to providers/prometheus Replace go-grpc-middleware v1 ChainUnary/StreamServer/Client with local chain helpers. Remove grpc_ctxtags (trace ID already propagated via loggers). Update grpc_retry to v2 module path. Migrate go-grpc-prometheus to go-grpc-middleware/providers/prometheus with instance-based metrics API. New exported functions SetServerMetricsOptions and SetClientMetricsOptions allow configuring gRPC Prometheus metrics with any grpcprom option during initialization. Metric names are preserved (grpc_server_handled_total, grpc_server_handling_seconds, etc.). go-grpc-middleware v1 retained solely for grpc_opentracing (future removal).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded lazy-initialized Prometheus metrics builders with option setters, introduced internal interceptor chaining helpers for unary/stream client and server flows, swapped Prometheus provider dependency, simplified server error trace handling, and added unit tests for the chain helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ClientChain as Client Interceptor Chain
participant GRPC as gRPC Transport
participant ServerChain as Server Interceptor Chain
participant Server
participant PromReg as Prometheus Registry
Client->>ClientChain: start RPC
ClientChain->>PromReg: ensure client metrics registered (getClientMetrics)
ClientChain->>GRPC: invoke gRPC call
GRPC->>ServerChain: deliver request
ServerChain->>PromReg: ensure server metrics registered (getServerMetrics)
ServerChain->>Server: call handler
Server->>ServerChain: return response / error (trace id attached)
ServerChain->>GRPC: return response
GRPC->>ClientChain: deliver response
ClientChain->>Client: return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interceptors.go`:
- Around line 531-536: The stream interceptor enriches ctx via
notifier.SetTraceId and loggers.AddToLogContext but still passes the original
grpc.ServerStream to handler, so downstream callers see the old context; wrap
the incoming stream in a small wrapper type (e.g. serverStreamWithContext
implementing grpc.ServerStream and overriding Context() to return the enriched
ctx) and pass that wrapped stream to handler instead of the original stream so
stream.Context() returns the new trace/log context; add the
serverStreamWithContext type and use it at the call site where handler(srv,
stream) is invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c048cf28-a9d9-45aa-81d8-80e15744c404
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
README.mdgo.modinterceptors.go
There was a problem hiding this comment.
Pull request overview
This PR migrates the interceptor stack off deprecated go-grpc-middleware v1 chaining and go-grpc-prometheus, moving to go-grpc-middleware/v2 retry and the instance-based Prometheus provider while keeping existing metric names.
Changes:
- Replaced v1
Chain*helpers with local chaining helpers for unary server/client and stream client interceptors. - Removed
grpc_ctxtagsusage and switched trace ID logging tologgers.AddToLogContext. - Migrated Prometheus instrumentation to
go-grpc-middleware/providers/prometheusand added metrics option setters for server/client.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| interceptors.go | Swaps chaining implementation, removes ctxtags, introduces singleton Prometheus metrics instances with configurable options. |
| go.mod | Replaces go-grpc-prometheus with providers/prometheus and adds go-grpc-middleware/v2. |
| go.sum | Updates dependency checksums for the new Prometheus provider and middleware v2. |
| README.md | Regenerated docs to reflect new/shifted APIs and line references. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… add chain tests - Register ServerMetrics/ClientMetrics with prometheus.DefaultRegisterer inside getServerMetrics/getClientMetrics (non-panicking Register with warning log on duplicate). Without registration, metrics from providers/prometheus are collected but never scraped. - Replace closure-per-RPC chain helpers with index-based runners that avoid per-call allocations. - Add TestChainUnaryServer, TestChainUnaryClient, TestChainStreamClient to verify interceptor execution ordering.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
interceptors.go (1)
542-559:⚠️ Potential issue | 🟠 MajorStream context enrichment not propagated to handler.
The
ctxis enriched with trace ID at lines 545-547, buthandler(srv, stream)at line 549 receives the originalstreamwhoseContext()method returns the unenriched context. Downstream handlers readingstream.Context()will not see the trace ID.This was flagged in a previous review. The fix requires wrapping the stream:
Proposed fix
+type serverStreamWithContext struct { + grpc.ServerStream + ctx context.Context +} + +func (s *serverStreamWithContext) Context() context.Context { + return s.ctx +}Then at the call site:
- err = handler(srv, stream) + err = handler(srv, &serverStreamWithContext{ServerStream: stream, ctx: ctx})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interceptors.go` around lines 542 - 559, The handler is called with the original grpc.ServerStream so its Context() doesn't include the added trace info; wrap the stream with a small type (e.g., streamWithContext or wrappedStream) that embeds grpc.ServerStream and overrides Context() to return the enriched ctx (the ctx produced after notifier.SetTraceId and loggers.AddToLogContext) and pass that wrapped stream into handler(srv, wrappedStream) inside ServerErrorStreamInterceptor so downstream handlers reading stream.Context() see the trace id.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@interceptors.go`:
- Around line 542-559: The handler is called with the original grpc.ServerStream
so its Context() doesn't include the added trace info; wrap the stream with a
small type (e.g., streamWithContext or wrappedStream) that embeds
grpc.ServerStream and overrides Context() to return the enriched ctx (the ctx
produced after notifier.SetTraceId and loggers.AddToLogContext) and pass that
wrapped stream into handler(srv, wrappedStream) inside
ServerErrorStreamInterceptor so downstream handlers reading stream.Context() see
the trace id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 10f55110-6c2a-429b-a4fc-25f7694e2648
📒 Files selected for processing (4)
README.mdgo.modinterceptors.gointerceptors_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- go.mod
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When consumers update interceptors without updating core, the old go-grpc-prometheus package's init() auto-registers metrics with the same names. Our prometheus.Register() then fails with AlreadyRegisteredError, leaving our metrics instance unregistered and unscraped. Fix: on AlreadyRegisteredError, unregister the old collector and re-register ours so the interceptors' metrics instance is the one being scraped.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interceptors.go`:
- Around line 127-128: The doc comment for SetClientMetricsOptions is incorrect
about lifecycle; update it to state that SetClientMetricsOptions configures
client-side interceptors and must be called during initialization before any
RPCs are made (not "before the server starts"), aligning its lifecycle guidance
with other client APIs; reference the SetClientMetricsOptions function and
mention it is not safe for concurrent use and should be invoked prior to
creating/using any gRPC clients.
- Around line 165-214: The three chain helpers (chainUnaryServer,
chainUnaryClient, chainStreamClient) use a shared mutable index `i` which breaks
re-entrancy when an interceptor calls next() multiple times; replace the current
incremental-i pattern with an index-capturing composition by building the
chained handler ahead of time (or using a recursive closure that captures the
current index) so each nested next closure closes over its own index instead of
a shared `i`; update chainUnaryServer, chainUnaryClient and chainStreamClient to
construct the composed handler/streamer/invoker by iterating from the end of the
interceptors slice (or via a recursive function param like idx) so retried calls
will run the full interceptor chain every time.
| // SetClientMetricsOptions appends gRPC client metrics options. | ||
| // Must be called during initialization, before the server starts. Not safe for concurrent use. |
There was a problem hiding this comment.
Fix the client metrics lifecycle docs.
SetClientMetricsOptions configures client-side interceptors, so “before the server starts” is the wrong lifecycle boundary. Please align this with the other client APIs and document that it must run before any RPCs are made.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interceptors.go` around lines 127 - 128, The doc comment for
SetClientMetricsOptions is incorrect about lifecycle; update it to state that
SetClientMetricsOptions configures client-side interceptors and must be called
during initialization before any RPCs are made (not "before the server starts"),
aligning its lifecycle guidance with other client APIs; reference the
SetClientMetricsOptions function and mention it is not safe for concurrent use
and should be invoked prior to creating/using any gRPC clients.
…ict detection Add compile-time version compatibility sentinel following the gRPC SupportPackageIsVersion pattern. Downstream packages (core) reference this constant to enforce exact version compatibility. Update registerCollector to use errors.As for AlreadyRegisteredError detection. When descriptor conflicts occur (e.g. go-grpc-prometheus init() registered same metric names), log a clear error directing users to update core.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
interceptors.go (2)
169-218:⚠️ Potential issue | 🟠 MajorMake chain helpers re-entrant (shared index currently breaks repeated
nextcalls).All three helpers mutate a shared
i. If any interceptor invokesnextmore than once, subsequent invocations skip part of the chain.Proposed fix (index-capturing recursion)
func chainUnaryServer(interceptors []grpc.UnaryServerInterceptor) grpc.UnaryServerInterceptor { return func(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) { - var i int - var next grpc.UnaryHandler - next = func(ctx context.Context, req interface{}) (interface{}, error) { - if i == len(interceptors) { - return handler(ctx, req) - } - interceptor := interceptors[i] - i++ - return interceptor(ctx, req, info, next) - } - return next(ctx, req) + var call func(int, context.Context, interface{}) (interface{}, error) + call = func(i int, ctx context.Context, req interface{}) (interface{}, error) { + if i == len(interceptors) { + return handler(ctx, req) + } + return interceptors[i](ctx, req, info, func(nextCtx context.Context, nextReq interface{}) (interface{}, error) { + return call(i+1, nextCtx, nextReq) + }) + } + return call(0, ctx, req) } } func chainUnaryClient(interceptors []grpc.UnaryClientInterceptor) grpc.UnaryClientInterceptor { return func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, invoker grpc.UnaryInvoker, opts ...grpc.CallOption) error { - var i int - var next grpc.UnaryInvoker - next = func(ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, opts ...grpc.CallOption) error { - if i == len(interceptors) { - return invoker(ctx, method, req, reply, cc, opts...) - } - interceptor := interceptors[i] - i++ - return interceptor(ctx, method, req, reply, cc, next, opts...) - } - return next(ctx, method, req, reply, cc, opts...) + var call func(int, context.Context, string, interface{}, interface{}, *grpc.ClientConn, ...grpc.CallOption) error + call = func(i int, ctx context.Context, method string, req, reply interface{}, cc *grpc.ClientConn, opts ...grpc.CallOption) error { + if i == len(interceptors) { + return invoker(ctx, method, req, reply, cc, opts...) + } + return interceptors[i](ctx, method, req, reply, cc, func(nextCtx context.Context, nextMethod string, nextReq, nextReply interface{}, nextCC *grpc.ClientConn, nextOpts ...grpc.CallOption) error { + return call(i+1, nextCtx, nextMethod, nextReq, nextReply, nextCC, nextOpts...) + }, opts...) + } + return call(0, ctx, method, req, reply, cc, opts...) } } func chainStreamClient(interceptors []grpc.StreamClientInterceptor) grpc.StreamClientInterceptor { return func(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, streamer grpc.Streamer, opts ...grpc.CallOption) (grpc.ClientStream, error) { - var i int - var next grpc.Streamer - next = func(ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, opts ...grpc.CallOption) (grpc.ClientStream, error) { - if i == len(interceptors) { - return streamer(ctx, desc, cc, method, opts...) - } - interceptor := interceptors[i] - i++ - return interceptor(ctx, desc, cc, method, next, opts...) - } - return next(ctx, desc, cc, method, opts...) + var call func(int, context.Context, *grpc.StreamDesc, *grpc.ClientConn, string, ...grpc.CallOption) (grpc.ClientStream, error) + call = func(i int, ctx context.Context, desc *grpc.StreamDesc, cc *grpc.ClientConn, method string, opts ...grpc.CallOption) (grpc.ClientStream, error) { + if i == len(interceptors) { + return streamer(ctx, desc, cc, method, opts...) + } + return interceptors[i](ctx, desc, cc, method, func(nextCtx context.Context, nextDesc *grpc.StreamDesc, nextCC *grpc.ClientConn, nextMethod string, nextOpts ...grpc.CallOption) (grpc.ClientStream, error) { + return call(i+1, nextCtx, nextDesc, nextCC, nextMethod, nextOpts...) + }, opts...) + } + return call(0, ctx, desc, cc, method, opts...) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interceptors.go` around lines 169 - 218, The chain helpers (chainUnaryServer, chainUnaryClient, chainStreamClient) use a shared mutable index `i`, so if any interceptor calls `next` more than once the second call skips interceptors; fix by making the recursion index-local (capture the index per invocation) — replace the shared `i`/single `next` pattern with a recursive function that takes an index parameter (e.g. chain(idx int) or nextAt(index int)) and invokes the appropriate interceptor or final handler/invoker/streamer using that index, incrementing the index only for the recursive call; apply the same index-capturing recursion change to the UnaryServer, UnaryClient (use invoker), and StreamClient (use streamer) helpers so repeated `next` calls traverse the chain correctly.
133-135:⚠️ Potential issue | 🟡 MinorCorrect
SetClientMetricsOptionslifecycle docs.This is a client-side configuration API, so “before the server starts” is the wrong boundary. It should say it must be called during init before any RPCs are made.
Proposed doc fix
// SetClientMetricsOptions appends gRPC client metrics options. -// Must be called during initialization, before the server starts. Not safe for concurrent use. +// Must be called during initialization, before any RPCs are made. Not safe for concurrent use. func SetClientMetricsOptions(opts ...grpcprom.ClientMetricsOption) { cltMetricsOpts = append(cltMetricsOpts, opts...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interceptors.go` around lines 133 - 135, Update the doc comment for SetClientMetricsOptions to reflect its client-side lifecycle: replace “Must be called during initialization, before the server starts. Not safe for concurrent use.” with a line stating it must be called during initialization before any RPCs are made and is not safe for concurrent use, so callers should configure it prior to making client calls; reference the function name SetClientMetricsOptions in the comment so it's clear which API is affected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interceptors.go`:
- Around line 149-150: The current log.Error call in interceptors.go (the
log.Error(..., "gRPC Prometheus metrics will not be available: the deprecated
go-grpc-prometheus package has already registered metrics with the same names.
Update github.com/go-coldbrew/core to the latest version to fix this.", "err",
err)) is misleading for non-AlreadyRegistered errors; update that message to a
neutral, accurate statement (e.g., "gRPC Prometheus metrics registration
failed") and include the actual error details (err) in the log so callers see
the real cause instead of incorrectly blaming the deprecated package. Ensure the
change is applied to the log.Error invocation that uses the err variable so
debugging shows the error text.
---
Duplicate comments:
In `@interceptors.go`:
- Around line 169-218: The chain helpers (chainUnaryServer, chainUnaryClient,
chainStreamClient) use a shared mutable index `i`, so if any interceptor calls
`next` more than once the second call skips interceptors; fix by making the
recursion index-local (capture the index per invocation) — replace the shared
`i`/single `next` pattern with a recursive function that takes an index
parameter (e.g. chain(idx int) or nextAt(index int)) and invokes the appropriate
interceptor or final handler/invoker/streamer using that index, incrementing the
index only for the recursive call; apply the same index-capturing recursion
change to the UnaryServer, UnaryClient (use invoker), and StreamClient (use
streamer) helpers so repeated `next` calls traverse the chain correctly.
- Around line 133-135: Update the doc comment for SetClientMetricsOptions to
reflect its client-side lifecycle: replace “Must be called during
initialization, before the server starts. Not safe for concurrent use.” with a
line stating it must be called during initialization before any RPCs are made
and is not safe for concurrent use, so callers should configure it prior to
making client calls; reference the function name SetClientMetricsOptions in the
comment so it's clear which API is affected.
Reference upstream errors package sentinel to enforce version compatibility at compile time.
…rtPackageIsVersion1)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
go.mod (1)
187-187: Movegomarkdocfrom indirect to direct tooling requirement.
gomarkdocis explicitly listed in thetool (...)block (lines 278–282), making it a direct tooling dependency. Remove the// indirectmarker on line 187 to follow the Go convention that explicitly required tools remain as direct requires.Proposed go.mod adjustment
- github.com/princjef/gomarkdoc v1.1.0 // indirect + github.com/princjef/gomarkdoc v1.1.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 187, The go.mod currently marks github.com/princjef/gomarkdoc as an indirect dependency; update the requirement so gomarkdoc is a direct tooling requirement by removing the "// indirect" marker for github.com/princjef/gomarkdoc v1.1.0 and ensuring it remains declared in the tool (...) block; edit the go.mod entry for the module name "github.com/princjef/gomarkdoc" to remove the indirect comment so it is treated as a direct tooling requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@go.mod`:
- Line 187: The go.mod currently marks github.com/princjef/gomarkdoc as an
indirect dependency; update the requirement so gomarkdoc is a direct tooling
requirement by removing the "// indirect" marker for
github.com/princjef/gomarkdoc v1.1.0 and ensuring it remains declared in the
tool (...) block; edit the go.mod entry for the module name
"github.com/princjef/gomarkdoc" to remove the indirect comment so it is treated
as a direct tooling requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a88d003b-cd14-46d0-b372-37f70ee441b1
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.modinterceptors.go
🚧 Files skipped from review as they are similar to previous changes (1)
- interceptors.go
Replace shared mutable index with backwards iteration where each closure captures its own interceptor and next handler. Safe for concurrent use and if an interceptor calls next() multiple times. Add concurrent tests for all three chain helpers (server, client, stream client) that validate output correctness across 100 goroutines with -race enabled.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
go-grpc-middlewarev1ChainUnaryServer/Client/StreamClientwith local chain helpersgrpc_ctxtags— trace ID already propagated vialoggers.AddToLogContextgrpc_retrytogo-grpc-middleware/v2/interceptors/retrygo-grpc-prometheustogo-grpc-middleware/providers/prometheus(instance-based API)SetServerMetricsOptionsandSetClientMetricsOptionsfor configurable gRPC Prometheus metricsgrpc_server_handled_total,grpc_server_handling_seconds, etc.)go-grpc-middlewarev1 retained solely forgrpc_opentracing(future removal phase)Test plan
make test— all tests passmake lint— 0 issues, no vulnerabilitiesmake build— compiles cleanlymake doc— README regeneratedSummary by CodeRabbit
New Features
Documentation
Chores
Tests